Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(chain): Use network kind of TestnetKind in transparent addresses on Regtest #9175

Merged
merged 3 commits into from
Feb 7, 2025

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jan 28, 2025

Motivation

The non-finalized transparent transfer state is storing tx ids spending to or receiving from an address by getting addresses from outputs and assigning the node network's kind as the address network kind. This makes it difficult to query address tx ids or balances because Regtest transparent addresses use the same b58 address prefixes as Testnet, and addresses with the Regtest prefix are parsed as Testnet addresses in most of Zebra.

Closes #9161.

Solution

  • Avoid setting transparent address network kind as RegtestKind

Related:

  • Removes outdated TODO

Tests

Covered by Zaino's zebrad_send_to_transparent test (see note on modifications)

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@arya2 arya2 requested review from a team as code owners January 28, 2025 16:05
@arya2 arya2 requested review from upbqdn and removed request for a team January 28, 2025 16:05
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jan 28, 2025
@arya2 arya2 added C-bug Category: This is a bug I-usability Zebra is hard to understand or use C-testing Category: These are tests A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes P-High 🔥 and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Jan 28, 2025
@AloeareV
Copy link
Contributor

Follow-up Work

Investigate why the test is still panicking (in quick_shield() now).

On the zingolib side, I've tracked the error through to a call to librustzcash/zcash_client_backend::proto::service::compact_tx_streamer_client::CompactTxStreamerClient::send_transaction, which returns the following error on this commit, and is functional on the parent commit.

[/home/aloe/repos/zingo/zingolib/zingolib/src/grpc_connector.rs:362:9] client.send_transaction(request).await = Err(
    Status {
        code: Internal,
        message: "JsonRpcConnector error: Error: Serialization/Deserialization Error: missing field `result` at line 1 column 498",
        metadata: MetadataMap {
            headers: {
                "content-type": "application/grpc",
                "date": "Tue, 28 Jan 2025 19:29:26 GMT",
                "content-length": "0",
            },
        },
        source: None,
    },
)

@pacu
Copy link

pacu commented Jan 28, 2025

Reproduced the same on my side.

thread 'wallet_basic::zebrad_send_to_transparent' panicked at integration-tests/tests/wallet_to_validator.rs:173:49:
called `Result::unwrap()` on an `Err` value: CompleteAndBroadcast(Broadcast(Broadcast("Send Error: status: Internal, message: \"JsonRpcConnector error: Error: Serialization/Deserialization Error: missing field `result` at line 1 column 498\", details: [], metadata: MetadataMap { headers: {\"content-type\": \"application/grpc\", \"date\": \"Tue, 28 Jan 2025 21:52:20 GMT\", \"content-length\": \"0\"} }")))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test wallet_basic::zebrad_send_to_transparent ... FAILED

it seems that zingolib is expecting something like

{ "result": {....}} 

and got something really different ? (see at line 1 column 498\")

@mpguerra
Copy link
Contributor

mpguerra commented Feb 6, 2025

@mergify requeue

Copy link
Contributor

mergify bot commented Feb 6, 2025

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

mergify bot added a commit that referenced this pull request Feb 6, 2025
Copy link
Contributor

mergify bot commented Feb 6, 2025

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

@arya2
Copy link
Contributor Author

arya2 commented Feb 7, 2025

@Mergifyio update

Copy link
Contributor

mergify bot commented Feb 7, 2025

update

✅ Branch has been successfully updated

@mpguerra
Copy link
Contributor

mpguerra commented Feb 7, 2025

@mergify requeue

Copy link
Contributor

mergify bot commented Feb 7, 2025

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@mpguerra
Copy link
Contributor

mpguerra commented Feb 7, 2025

@mergify refresh

Copy link
Contributor

mergify bot commented Feb 7, 2025

refresh

✅ Pull request refreshed

@mpguerra
Copy link
Contributor

mpguerra commented Feb 7, 2025

@mergify refresh

Copy link
Contributor

mergify bot commented Feb 7, 2025

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit 97460cf into main Feb 7, 2025
144 of 145 checks passed
@mergify mergify bot deleted the fix-get-addr-tx-ids branch February 7, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces A-state Area: State / database changes C-bug Category: This is a bug C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-usability Zebra is hard to understand or use P-High 🔥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Non-Finalised State not returned by Zebra's ReadStateService in Regtest mode
7 participants